Make Subject required by default in NWBFile::initialize() by Copilot · Pull Request #320 · NeurodataWithoutBorders/aqnwb

Copilot · 2026-08-13T04:50:00Z

Fixes #290

nwb-inspector upgraded check_subject_exists to a warning/error for DANDI compliance. AqNWB did not yet support Subject and was silently ignoring this check in CI via --ignore=check_subject_exists.

Changes

Added Subject class to represent the /general/subject group in NWB files.
Added Subject::SubjectSpec to configure a Subject and facilitate initialization also via NWBFile.
Updated nwbinspector tests to remove --ignore=check_subject_exists option to require subject.
Updated NWBFile::initialize to accept a SubjectSpec argument for subject metadata initialization

Co-authored-by: oruebel <10999845+oruebel@users.noreply.github.com>

- Add SubjectMetadata struct to NWBFile.hpp with optional fields for species, sex, age, description, subjectId, genotype, strain, weight - Add subject parameter to NWBFile::initialize() defaulting to an empty SubjectMetadata{} (creates a Subject group by default); pass std::nullopt to explicitly opt out - Implement subject group creation in createFileStructure() with neurodata_type/namespace attributes for NWB compliance - Update testNWBFile.cpp to expect Subject in findOwnedTypes results - Update workflow example to demonstrate Subject usage with actual data - Remove --ignore=check_subject_exists from tests.yml and upgrade_schema.yml CI workflows Co-authored-by: oruebel <10999845+oruebel@users.noreply.github.com>

…thub.com/NeurodataWithoutBorders/aqnwb into copilot/make-subject-required-by-default

codecov-commenter · 2026-08-13T06:53:42Z

Codecov Report

❌ Patch coverage is 80.37383% with 21 lines in your changes missing coverage. Please review.
✅ Project coverage is 84.52%. Comparing base (c092a77) to head (76be639).

Files with missing lines	Patch %	Lines
src/nwb/file/Subject.cpp	74.07%	21 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main     #320      +/-   ##
==========================================
- Coverage   84.69%   84.52%   -0.18%     
==========================================
  Files          57       59       +2     
  Lines        2699     2805     +106     
  Branches      347      371      +24     
==========================================
+ Hits         2286     2371      +85     
- Misses        413      434      +21

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

Copilot

Pull request overview

This PR adds first-class support for the NWB /general/subject group and wires it into NWBFile::initialize() so CI nwbinspector validation no longer needs to ignore the subject-exists check, improving DANDI compliance.

Changes:

Added a new NWB::Subject container with a SubjectSpec for initializing subject metadata.
Extended NWBFile::initialize() to accept (optional) subject metadata and create /general/subject accordingly.
Updated tests, examples, and CI workflows to stop ignoring the subject-exists inspector check.

Reviewed changes

Copilot reviewed 17 out of 17 changed files in this pull request and generated 5 comments.

Show a summary per file

File	Description
tests/testUtils.hpp	Adds a shared helper to generate consistent `SubjectSpec` test metadata.
tests/testSubject.cpp	New unit tests covering Subject registration, read/write of fields, and integration with `NWBFile::initialize()`.
tests/testRegisteredType.cpp	Adds Subject to the registered type path/name expectations.
tests/testRecordingWorkflow.cpp	Updates workflow test to initialize NWBFile with Subject metadata.
tests/testProcessingModule.cpp	Updates processing module tests to initialize NWBFile with Subject metadata.
tests/testNWBFile.cpp	Updates NWBFile tests for the new initialize signature and Subject default creation expectations.
tests/examples/testWorkflowExamples.cpp	Updates example workflow to initialize NWBFile with Subject metadata.
tests/examples/test_link_timeseries_example.cpp	Updates link-timeseries example to initialize NWBFile with Subject metadata.
tests/CMakeLists.txt	Adds the new `testSubject.cpp` to the test build.
src/nwb/NWBFile.hpp	Extends `initialize()` API to accept `SubjectSpec` (via `std::optional`).
src/nwb/NWBFile.cpp	Implements Subject creation during NWBFile initialization.
src/nwb/file/Subject.hpp	Introduces the Subject container and `SubjectSpec` definition and read accessors.
src/nwb/file/Subject.cpp	Implements Subject initialization/writing of provided metadata.
CMakeLists.txt	Adds `Subject.cpp` to the library build.
CHANGELOG.md	Documents addition of Subject and nwbinspector/initialize updates.
.github/workflows/upgrade_schema.yml	Removes ignore of subject-exists; keeps ignore for Allen CCF electrodes location check.
.github/workflows/tests.yml	Removes ignore of subject-exists; keeps ignore for Allen CCF electrodes location check.

Suppressed comments (2)

src/nwb/NWBFile.hpp:113

The PR title/issue says Subject should be created by default (with an explicit opt-out), but initialize() currently defaults subjectSpec to std::nullopt, so callers who omit the argument still get no /general/subject and will fail nwbinspector unless they remember to pass metadata.

                    const std::string& timestampsReferenceTime = "",
                    const std::optional<AQNWB::NWB::Subject::SubjectSpec>&
                        subjectSpec = std::nullopt);

src/nwb/file/Subject.hpp:78

Leftover TODO suggests the initialize method is incomplete, but initialize() is already implemented in Subject.cpp. This looks like generated boilerplate that should be removed to avoid confusion.

  // TODO: Update the initialize method as appropriate.
  /**

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>

…by-default

… the spec

…by-default

rly · 2026-09-02T06:45:36Z

This PR appears not to create an empty Subject by default, contrary to the docs for initialize. No subject is created/added by default.

We could fix this by creating an empty Subject by default, but the file would still not pass DANDI validation because subject_id, species, sex, and age/date_of_birth are required.

Some options:

Drop the default on subjectSpec, making it a compile error to omit. Matches Make Subject required by default #290, breaks every existing caller loudly, which might be a good thing.
Keep the default and fix the docstring to say a subject is not created unless you pass one. Honest, but it should be made clear that these files cannot be uploaded to DANDI without a subject with required fields.
Add an overload taking (identifier, subjectSpec) so supplying a subject is the short call rather than the long one, which removes the ergonomic pressure toward omitting it: To supply a subject, a user must also spell out the four parameters in front of it.

…by-default

typed data facades transient, and document the revised factory behavior.

oruebel · 2026-09-02T22:00:13Z

2. Keep the default and fix the docstring to say a subject is not created unless you pass one. Honest, but it should be made clear that these files cannot be uploaded to DANDI without a subject with required fields.

I went with this option for now. Fixed in 8fcb541

Copilot

🟡 Changes recommended

NWBFile::initialize() still defaults to not creating a Subject (via std::nullopt), which conflicts with the stated requirement/title to make Subject required by default with an explicit opt-out.

Once you've addressed the issues Copilot identified, you can request another Copilot review.

Review details

Suppressed comments (2)

src/nwb/NWBFile.hpp:129

This initialize overload defaults subjectSpec to std::nullopt, meaning a Subject is not created unless the caller provides metadata. That conflicts with the PR title/issue requirement to make Subject required by default while allowing callers to explicitly opt out (e.g., by passing nullopt).

   * @param subjectSpec Optional subject metadata. Defaults to @c std::nullopt,
   * so no Subject group is created unless a SubjectSpec is supplied. For DANDI
   * validation, the supplied SubjectSpec must include subjectId, species, sex,
   * and either age or dateOfBirth.
   */
  Status initialize(const std::string& identifierText,
                    const std::string& description = "a recording session",
                    const std::string& dataCollection = "",
                    const std::string& sessionStartTime = "",
                    const std::string& timestampsReferenceTime = "",
                    const std::optional<AQNWB::NWB::Subject::SubjectSpec>&
                        subjectSpec = std::nullopt);

CHANGELOG.md:50

Changelog entry has a consistent typo in the API name: DyanmicTable should be DynamicTable (twice on this line).

   * Added `DyanmicTable::validateDataSpecs` (and `DynamicTable::checkRequiredColumnNames` helper function) to validate column specifications as part of `DyanmicTable::initialize` before initialization. Also updated subclasses of `DynamicTable` (e.g., `ElectrodesTable`, `EventsTable`, `MeaningsTable`) to override `validateDataSpecs` to provide their specific validation logic. (@oruebel, [#305](https://github.com/NeurodataWithoutBorders/aqnwb/pull/305))

Files reviewed: 30/30 changed files
Comments generated: 3
Review effort level: Lite

+   * Added tutorials for the new `MeaningsTable` and `EventsTable` types in `docs/pages/userdocs/events.dox`. (@cline, @oruebel, [#305](https://github.com/NeurodataWithoutBorders/aqnwb/pull/305))
+   * Added tutorials for the new `TimeIntervals` type in `docs/pages/userdocs/time_intervals.dox`. (@cline, @oruebel, [#325](https://github.com/NeurodataWithoutBorders/aqnwb/pull/325))
+   * Added `Subject` class to represent the `/general/subject` group in NWB files. Added corresponding `SubjectSpec` to simplify configuration and initialization of `Subject`.  Updated `NWBFile::initialize` to accept a `SubjectSpec` argument for subject metadata initialization (@copilot, @oruebel, [#320](https://github.com/NeurodataWithoutBorders/aqnwb/pull/320))
+   * Added `isPathOrDescendant` utility functoin to simplify path initialization checks for root groups, e.g, `/events` and `/intervals` (@copilot, @oruebel, [#325](https://github.com/NeurodataWithoutBorders/aqnwb/pull/325))


 * **[BREAKING]** Moved `disableSWMRMode` option from `HDF5IO` constructor to a new `HDF5IO::startRecording(bool disableSWMRMode)` overload. The `BaseIO`-compliant `startRecording()` override is preserved and defaults to SWMR enabled. 
   * **Migration Note**: Code using `HDF5IO(path, true)` must be updated to `HDF5IO(path)` followed by `startRecording(true)`. When the `HDF5IO` object is held as a `std::shared_ptr<BaseIO>` (e.g., from `createIO`), downcast with `std::dynamic_pointer_cast<HDF5IO>` to access the overload. (@oruebel [#297](https://github.com/NeurodataWithoutBorders/aqnwb/pull/297))
-* **[BREAKING]** Refactored `DynamicTable` so it creates and initializes its `ElementIdentifiers` `id` column internally during `initialize()`. As part of this change, `setRowIDs()` now takes only the row ID values and writes them directly to the table's built-in `id` column instead of accepting a separate `ElementIdentifiers` object. In practice this aligns the API with the intended usage, since callers should have always used the table's own `id` dataset. (@oruebel, [#302](https://github.com/NeurodataWithoutBorders/aqnwb/pull/304))
+* **[BREAKING]** Updated `Types.hpp` to use a `namespace` instead of `class` to group types. (@oruebel, [#325]((https://github.com/NeurodataWithoutBorders/aqnwb/pull/325))


+  /** \brief Convenience factor method since the path is fixed to
+   * '/general/subject'
+   * @param io A shared pointer to the IO object.
+   * @return A shared pointer to the created NWBFile object, or nullptr if
+   * creation failed.
+   */


Initial plan

689d747

Copilot AI assigned Copilot and oruebel Aug 13, 2026

Copilot started work on behalf of oruebel August 13, 2026 04:50 View session

Copilot AI linked an issue Aug 13, 2026 that may be closed by this pull request

Make Subject required by default #290

Closed

Copilot AI and others added 2 commits August 13, 2026 04:58

Apply remaining changes

54fd124

Co-authored-by: oruebel <10999845+oruebel@users.noreply.github.com>

Copilot AI changed the title ~~[WIP] Update NWBFile to require subject metadata by default~~ Make Subject required by default in NWBFile::initialize() Aug 13, 2026

Copilot finished work on behalf of oruebel August 13, 2026 05:06

Copilot AI requested a review from oruebel August 13, 2026 05:06

oruebel added 5 commits August 12, 2026 23:18

Added Subject type

b4bd01e

Merge branch 'copilot/make-subject-required-by-default' of https://gi…

ac90401

…thub.com/NeurodataWithoutBorders/aqnwb into copilot/make-subject-required-by-default

Update unit tests

d5a741e

Update Changelog and fix docstring

a526662

Add missing testSubject.cpp file

b732aea

oruebel added 4 commits August 13, 2026 00:08

Fix nwb-inspector validation errors

dabddc7

Fix validation tests

cbd9a1f

Fix nwb-inspector validation errors

10de7dd

Fix nwb-inspector validation errors

bcb223e

oruebel mentioned this pull request Aug 13, 2026

Fix support for electrode location #321

Open

Fix subject metadata nwb-inspector test

d3f1893

oruebel approved these changes Aug 13, 2026

View reviewed changes

oruebel marked this pull request as ready for review August 13, 2026 08:56

Copilot AI lite review requested due to automatic review settings August 13, 2026 08:56

Copilot started reviewing on behalf of oruebel August 13, 2026 08:57 View session

oruebel requested a review from rly August 13, 2026 09:02

Copilot AI reviewed Aug 13, 2026

View reviewed changes

Comment thread src/nwb/NWBFile.cpp

Comment thread src/nwb/file/Subject.hpp Outdated

Comment thread src/nwb/file/Subject.cpp Outdated

Comment thread src/nwb/NWBFile.hpp Outdated

Comment thread src/nwb/file/Subject.cpp

oruebel and others added 2 commits August 13, 2026 02:05

Potential fix for pull request finding

49f9ea6

Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>

Potential fix for pull request finding

b016125

Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>

oruebel and others added 9 commits August 21, 2026 17:02

Merge branch 'add_time_intervals' into copilot/make-subject-required-…

d262d8b

…by-default

Fix failing unit test

526e6f5

Fix NWB scherma version in change log

6d04453

Fix column docstrings in EventsTable::createDefaultDataSpecs to match…

1f64903

… the spec

Update MeaningsTable.target doc

2d43100

Merge branch 'add_time_intervals' into copilot/make-subject-required-…

35d2eaf

…by-default

Update changelog to fix formatting

907d4f9

Fix bad link in CHANGLEOG

dd449bb

Merge branch 'add_time_intervals' into copilot/make-subject-required-…

07deef1

…by-default

oruebel mentioned this pull request Sep 2, 2026

Update schema to NWB 2.11 #300

Merged

62 tasks done

rly reviewed Sep 2, 2026

View reviewed changes

Comment thread .github/workflows/tests.yml

rly reviewed Sep 2, 2026

View reviewed changes

Comment thread src/nwb/file/Subject.cpp Outdated

oruebel added 6 commits September 2, 2026 12:53

Updated changelog

7c3630c

Update changelog

64ee4a6

Merge branch 'add_time_intervals' into copilot/make-subject-required-…

59a9718

…by-default

Ensure cached registered objects match their concrete on-disk type, keep

616b39e

typed data facades transient, and document the revised factory behavior.

Fix status checks in the Subject class

5131191

Fix NWBFile::initialize() docstring to clarify subject creation

8fcb541

rly approved these changes Sep 2, 2026

View reviewed changes

oruebel requested a lite review from Copilot September 3, 2026 00:08

Copilot started reviewing on behalf of oruebel September 3, 2026 00:09 View session

oruebel merged commit 71c97c3 into add_time_intervals Sep 3, 2026
17 checks passed

oruebel deleted the copilot/make-subject-required-by-default branch September 3, 2026 00:09

Copilot AI reviewed Sep 3, 2026

View reviewed changes

Navigation Menu

Repository navigation

Make Subject required by default in NWBFile::initialize() - #320

Make Subject required by default in NWBFile::initialize()#320
oruebel merged 50 commits into
add_time_intervalsfrom
copilot/make-subject-required-by-default

Copilot AI commented Aug 13, 2026 •

edited by oruebel

Loading

Uh oh!

codecov-commenter commented Aug 13, 2026 •

edited

Loading

Uh oh!

Copilot AI left a comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

rly commented Sep 2, 2026

Uh oh!

oruebel commented Sep 2, 2026

Uh oh!

Uh oh!

Copilot AI left a comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

Conversation

Copilot AI commented Aug 13, 2026 • edited by oruebel Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Changes

Uh oh!

codecov-commenter commented Aug 13, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Reviewed changes

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

rly commented Sep 2, 2026

Uh oh!

oruebel commented Sep 2, 2026

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

🟡 Changes recommended

Suppressed comments (2)

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

Copilot AI commented Aug 13, 2026 •

edited by oruebel

Loading

codecov-commenter commented Aug 13, 2026 •

edited

Loading